Skip to content

fix(jobs): atomic dot-path writes for file_urls/state_data (lost-update race) - #970

Merged
beveradb merged 2 commits into
mainfrom
feat/sess-20260831-1800-fix-file-urls-race
Aug 31, 2026
Merged

fix(jobs): atomic dot-path writes for file_urls/state_data (lost-update race)#970
beveradb merged 2 commits into
mainfrom
feat/sess-20260831-1800-fix-file-urls-race

Conversation

@beveradb

Copy link
Copy Markdown
Collaborator

Summary

  • JobManager.update_file_url and update_state_data copied the whole file_urls/state_data map and re-persisted it. The audio and screens workers run in parallel and both write these maps, so a write built from a stale snapshot silently dropped a sibling key set in between (a classic lost update).
  • Real symptom (job 5588e466, Keeno "Shelter from the Storm"): backing_vocals.flac was uploaded to GCS but missing from file_urls.stems (only the 4th-of-5 stem lost). The instrumental-review UI couldn't load the backing preview, and _analyze_backing_vocals early-returned → "Very few backing vocals detected (0%)" → the C1 mirror-skip forwarded /review/instrumental.
  • Rare — needs a precise interleaving (~1 in 4 parallel jobs). Pre-existing; unrelated to the timing-gate work.

Changes

  • update_file_url / update_state_data now write a single Firestore dot-path field (file_urls.<cat>.<type>, state_data.<key>) so siblings merge server-side — matching the existing update_processing_metadata pattern. All keys are plain snake_case (no dots), so no path is misread as nesting.
  • screens_worker._analyze_backing_vocals hardened: recovers a stem present at the conventional GCS path but missing from file_urls (re-registers it atomically), and stores a fallback analysis instead of a silent early-return.
  • Tests: unit tests asserting dot-path writes; emulator regression tests (test_file_urls_no_clobber.py) that reproduce the lost update via a stale snapshot — they fail against the old read-modify-write and pass against the fix. Updated one assertion in test_job_manager.py and the missing-stems worker test for the new behavior.
  • docs/LESSONS-LEARNED.md entry; version bump 0.216.0 → 0.216.1.

Testing

  • Full backend unit suite green (4216 passed; the only failures were emulator-connection env issues, verified green once the emulator was started).
  • Emulator suite green incl. new regression tests (127 passed); new tests confirmed to fail on the old impl and pass on the new.
  • CodeRabbit CLI review — No findings.

Follow-up (not in this PR)

  • Remediate the already-stuck job 5588e466 after deploy via POST /api/admin/jobs/{job_id}/regenerate-screens (the fixed screens worker recovers + re-registers the stem and recomputes the analysis).

Review

  • Local CodeRabbit review completed — no findings
  • Feedback addressed

@coderabbitai ignore


🤖 Generated with Claude Code

beveradb and others added 2 commits August 31, 2026 18:12
…te race)

update_file_url and update_state_data copied the whole map and re-persisted
it, so the parallel audio + screens workers could clobber each other's writes.
Real symptom: a backing_vocals stem uploaded to GCS but dropped from
file_urls.stems, so the instrumental-review UI couldn't load the backing
preview and _analyze_backing_vocals reported 0% backing.

- Write single Firestore dot-path fields (file_urls.<cat>.<type>,
  state_data.<key>) so siblings merge server-side, matching
  update_processing_metadata.
- _analyze_backing_vocals recovers a stem present at the conventional GCS
  path but missing from file_urls, and stores a fallback analysis instead of
  a silent early-return.
- Unit tests for dot-path writes + emulator regression tests that fail against
  the old read-modify-write and pass against the fix.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…00-fix-file-urls-race

# Conflicts:
#	pyproject.toml
@beveradb
beveradb enabled auto-merge (squash) August 31, 2026 23:44
@beveradb
beveradb merged commit 9616342 into main Aug 31, 2026
22 checks passed
beveradb added a commit that referenced this pull request Sep 2, 2026
…it races) (#974)

## Summary
Follow-up hardening sweep to the `file_urls`/`state_data` lost-update
fix (#970). An audit found the same **read-copy-mutate-write-whole-map**
pattern in 8 more sites; this converts them to atomic Firestore dot-path
writes so a stale snapshot can't clobber a concurrent sibling key.

## Changes
- **`video_worker` distribution / redistribution / finalize (3 sites)**
— the most dangerous: they rewrote the whole `state_data` map and
`.pop('visibility_change_in_progress')`, so a stale write could **revert
the `worker_generation` supersession fence** (an atomic `Increment`) or
resurrect the visibility guard — the "visibility-recycle-dataloss"
surface. Now dot-path writes, with `DELETE_FIELD` for the guard.
- **Audio-editor undo / redo / apply (`review.py`)** — full-map rewrites
that could race with rapid clicks or the idle-reminder scheduler active
during audio-edit → dot-path writes.
- **Audio-editor upload-for-join** — now writes only *this* upload's
entry via `FieldPath(...).to_api_repr()` (backtick-escapes the
hyphenated uuid, string key), so concurrent uploads **merge** instead of
one dropping the other (which orphaned a GCS file and 404'd on join).
*(CodeRabbit finding #1.)*
- **Deferred YouTube-URL write (`youtube_queue_processor`)** → dot-path.

## Out of scope (documented)
- The two audio-edit **list** fields (`audio_edit_stack` /
`audio_edit_redo_stack`) remain last-write-wins across concurrent edits
to the *same* job — a list can't be dot-path-merged. Acceptable for the
single-reviewer sequential audio-editor UI; true multi-writer safety
needs a transaction/CAS. Documented inline. *(CodeRabbit finding #2 —
deliberately deferred.)*
- Two low-risk fresh-dict-*replace* sites (create-from-search / admin
re-search) — single-request, early-lifecycle — left as-is.

## Testing
- [x] Full backend unit suite green (4266 passed) + emulator suite green
(118 passed).
- [x] New emulator regression tests: distribution write preserves the
`worker_generation` fence + clears the visibility guard (with a contrast
test showing the old full-map write reverted the fence); concurrent
audio-edit uploads both survive.
- [x] Updated the 2 unit tests that asserted the old full-map shape.
- [x] CodeRabbit CLI: finding #1 fixed, finding #2 documented/deferred;
no new findings on re-review.

## Review
- [x] Local CodeRabbit review completed (2 cycles); feedback addressed

@coderabbitai ignore

---
🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant